Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to change color of magnifier icon #3708

Merged
merged 3 commits into from
Feb 15, 2018
Merged

Try to change color of magnifier icon #3708

merged 3 commits into from
Feb 15, 2018

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Feb 7, 2018

I am trying to change the color of the magnifier icon as requested here: #3535

However, the icon refuses a color change. It even opts out of the color scheme of JabRef, since it is black instead of purple (despite the fact that the icon is configured as purple in the code).

@tobiasdiez How can I change the color of this icon? Any change will do, really. For reasons unknown to me the icon straight-out refuses the change.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@koppor koppor changed the title Try to change color of magnifier icon [WIP] Try to change color of magnifier icon Feb 7, 2018
@@ -13,7 +15,21 @@
public static TextField create() {
CustomTextField textField = (CustomTextField) TextFields.createClearableTextField();
textField.setPromptText(Localization.lang("Search") + "...");
textField.setLeft(IconTheme.JabRefIcon.SEARCH.getGraphicNode());
Node node = IconTheme.JabRefIcon.SEARCH.getGraphicNode();
node.setStyle("-fx-text-fill: #00ffff");
Copy link
Member

@tobiasdiez tobiasdiez Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-fx-fill should work, but do you really think that changing the icon color is a good idea? How intuitive (and important) is it to signal an advanced search mode by magenta?

The "right" way to change the style depending on the state in javafx are pseudo classes (so you can decouple the view from the style), see e.g. http://www.guigarage.com/2016/02/javafx-and-css-pseudo-classes/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, fx-fill doesn't help. There seems to be a deeper problem with exactly this icon.

Actually, I am quite indifferent to this, but @koppor really wants it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the proposal at #3535 (comment). However, JabRef had this functionality for at least one year. Animation at #3535 (comment). - Simon and I had a long discussion on that. The decision was that we should present to the user that there are different search modes. They should "just work", but for the pros, there should be a hint which mode is active. Not that someone wonders why the search does not work. -- Furthermore, that icon makes it less hard to debug the search. It is clear, whether the search term is a grammar-based search or just plain text string search.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I see your point. It is just again a feature that needs documentation before anybody understands what's going it.

Butt if fx-fill and fx-text-fill don't work, I'm running out of ideas. I usually use a combination out of both and it worked without any problems so far. Maybe it works if you base this PR on the maintable-beta branch. I changed a few things regarding javafx icons there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried some more (not the maintable branch, though) with no success. I think this is down to CustomTextField from controls-fx. It offers the functionality to add a node at the left or right side, but it seems to be impossible to change the style of this node.

Because of this, I've gone back to the previous version: A Swing icon next to the text field and everything just works. I know this is a step back, but if the CustomTextField cannot be made to use our icon theme, then we should probably migrate the whole GlobalSearchBar to JavaFX instead. Until this happens, I think it is wiser to use icons that preserve our color theme.

@tobiasdiez @koppor: I'll now leave it open to you to continue arguing, merge, or close this PR. I am fine with any solution.

@lenhard lenhard changed the title [WIP] Try to change color of magnifier icon Try to change color of magnifier icon Feb 13, 2018
@lenhard lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 13, 2018
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking out both versions (this PR and master) I prefer the master version. I think it's a problem when there's an icon in the toolbar that cannot be clicked.

@koppor
Copy link
Member

koppor commented Feb 14, 2018

Screenshot:
grafik

There is a space between the other icons. Until the cool solution from #3535 (comment) is implemented, I am in favor of using this approach. Reason: This makes the JabRef feature explicit.

papers search.

@tobiasdiez
Copy link
Member

Well, if @koppor really wants this I'm fine with it although I bet that more users get confused by a cyan-highlighted icon than they are getting enlighted by it.

@koppor koppor merged commit a20f70a into master Feb 15, 2018
@koppor koppor deleted the magnifier-color branch February 15, 2018 06:43
koppor added a commit that referenced this pull request Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants